Fix/array field validation #700

gutentag2012 · 2024-05-12T18:59:16Z

This PR closes #662

Any Array Field helper method now also runs the onChange validation of the Form and the field.
Furthermore, there is now a new method to validate a single field on a form.

…thods

… add missing moveFieldValues test

nx-cloud · 2024-05-18T07:23:36Z

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 43dff36. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution

✅ Successfully ran 1 target

nx affected --targets=test:format,test:sherif,test:knip,test:eslint,test:lib,test:types,test:build,build --parallel=3

Sent with 💌 from NxCloud.

packages/form-core/src/tests/FieldApi.spec.ts

Balastrong

A couple of questions:

We're now explicitly adding change validation on all methods, is there a reason why we're not adding it only to setFieldValue since all other methods are calling it anyway?
We're now running the validation on all changes which as a side effect sets touch: true on the field.
Does that mean that form.pushFieldValue("name", "", { touch: false }) would still set my field as touched due to the validation?

gutentag2012 · 2024-05-31T09:54:41Z

A couple of questions:

We're now explicitly adding change validation on all methods, is there a reason why we're not adding it only to setFieldValue since all other methods are calling it anyway?

We're now running the validation on all changes which as a side effect sets touch: true on the field.
Does that mean that form.pushFieldValue("name", "", { touch: false }) would still set my field as touched due to the validation?

I did that since I was unsure if validation is a wanted side-effect for the setFieldValue method. But I guess it would make sense to move the change validation there since that effectively is a handleChange method for the form
Oh yes you are right, that should not be the case. For the validateField method I copied the existing validation logic which sets the isTouched to true whenever validation runs. So in that case using form.pushFieldValue("name", "", { touch: false }) would validate and set the isTouched=true either way

I will fix 2, as for 1 I am open to move validation to the setFieldValue if that is wanted

Balastrong · 2024-05-31T10:02:22Z

I did that since I was unsure if validation is a wanted side-effect for the setFieldValue method. But I guess it would make sense to move the change validation there since that effectively is a handleChange method for the form

Ok! Just wanted to make sure this was intentional. Let's also wait for more reviews before doing any change here as it might be ok this way.

Oh yes you are right, that should not be the case. For the validateField method I copied the existing validation logic which sets the isTouched to true whenever validation runs. So in that case using form.pushFieldValue("name", "", { touch: false }) would validate and set the isTouched=true either way

I will fix 2, as for 1 I am open to move validation to the setFieldValue if that is wanted

Great, thanks!

gutentag2012 · 2024-05-31T10:02:42Z

How do we want to handle the following case:

We have a form with an onChange validation and do a form.pushFieldValue("name", "", { touch: false }). That should still run the validation on the form, since the form has no touched state for validation correct?

Balastrong · 2024-05-31T16:32:26Z

How do we want to handle the following case:

We have a form with an onChange validation and do a form.pushFieldValue("name", "", { touch: false }). That should still run the validation on the form, since the form has no touched state for validation correct?

I see you removed setting touched: true on validation, I think that was correct though. Since validateAllFields also sets touched, the single validation should do it too.

My point was more about pushFieldValue (and the other methods): should we run the validation if you pass { touched: false }?
If the answer is yes, your code is ok (by adding back the part you removed) but if the answer is no, we should also apply this condition to field.setValue since it is always running the validation now.

gutentag2012 · 2024-05-31T18:06:09Z

I see you removed setting touched: true on validation, I think that was correct though. Since validateAllFields also sets touched, the single validation should do it too.

I think it is better that way, because now validation only runs if the fields have been touched either through the { touch: true } option or because they have been touched before.
Having the validation method implicitly setting the touched state feels a bit off to me

My point was more about pushFieldValue (and the other methods): should we run the validation if you pass { touched: false }?

To answer your question, no I don't think validation should run if the field should not be touched (at least the field validation, the form validation should still run)

By keeping the code as it is now I do the same thing as the field.SetValue function, since that first sets the isTouched state depending on the options and then runs validation. If it was touched during that process, the validation will abort early.
This is the same as the array field helpers are doing it now.

fulopkovacs

I think it is better that way, because now validation only runs if the fields have been touched either through the { touch: true } option or because they have been touched before.
Having the validation method implicitly setting the touched state feels a bit off to me

@gutentag2012 We talked about the {touch: boolean} API among the maintainers because it's kind of confusing. The thing is, it's an API that is supposed to be used internally for testing. Right now this is not obvious at all, but in the future, we're planning to default it to true everywhere and rename it to something else.

I think we should go back to following the example of validateAllFields() for now and set isTouched to true for the validated field. This will also spare us from running the validate() method of the form where validate a field, because a field's validate() method will run the form's validate() function anyways.

So this this.validate('change') will become redundant:

form/packages/form-core/src/FormApi.ts

Lines 708 to 709 in de8d8aa

    
           this.validate('change') 
        
           this.validateField(field, 'change')

because the field's validate() method is called here:

form/packages/form-core/src/FormApi.ts

Line 381 in de8d8aa

return fieldInstance.validate(cause)

and it runs validate() on the form if isTouched is true for the field:

form/packages/form-core/src/FieldApi.ts

Lines 716 to 720 in de8d8aa

    
           if (!this.state.meta.isTouched) return [] 
        
           try { 
        
             this.form.validate(cause) 
        
           } catch (_) {}

Thanks for your patience and hard work btw! 🙌 If this issue gets resolved, we can merge the PR!

Notes on testing

The tests will need to be updated after this change, see how 👇

Note that if you don't run the form's validate() function explicitly, you'll need to create a field instance and mount it for the new tests, otherwise they'll fail. For example, the "should run onChange validation when pushing an array field's value" test would look like this:

  it("should run onChange validation when pushing an array field's value", () => {
    const form = new FormApi({
      defaultValues: {
        names: ['test'],
      },
      validators: {
        onChange: ({ value }) =>
          value.names.length > 3 ? undefined : 'At least 3 names are required',
      },
    })

    const field = new FieldApi({
      name: 'names',
      form,
    })

    form.mount()
    field.mount()
    form.pushFieldValue('names', 'other')

    expect(form.state.values).toStrictEqual({ names: ['test', 'other'] })
    expect(form.state.errors).toStrictEqual(['At least 3 names are required'])
  })

gutentag2012 · 2024-06-02T09:17:43Z

Oh, I see, that does make sense.
I will make the validateField method then also set the touched state and remove the explicit this.validate("change") for the form.

I think there might be an issue with the validation for the insertFieldValue and the swaptFieldValues and moveFieldValues. I will verify that and fix that if that is the case.

…ven index

gutentag2012 · 2024-06-02T10:37:34Z

Okay, so there was a bigger issue where nested fields within the arrays where not validated properly and removeFieldValue validating all fields within the form despite that not being necessary.

So I added a new method validateArrayFieldsStartingFrom to validate nested fields within an array starting from a specified index, that is now used in the removeFieldValue and insertFieldValue methods, since they required all shifted fields to revalidate the validation.

codecov · 2024-06-02T12:09:05Z

Codecov Report

Attention: Patch coverage is 97.50000% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 90.36%. Comparing base (8e2335e) to head (43dff36).
Report is 27 commits behind head on main.

Files	Patch %	Lines
packages/form-core/src/FormApi.ts	97.36%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #700      +/-   ##
==========================================
+ Coverage   89.95%   90.36%   +0.41%     
==========================================
  Files          32       32              
  Lines         896      955      +59     
  Branches      195      206      +11     
==========================================
+ Hits          806      863      +57     
- Misses         84       86       +2     
  Partials        6        6

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

fulopkovacs

Thanks a lot for your work, I'm merging it! 🚀

gutentag2012 added 8 commits May 12, 2024 20:04

fix(FormApi): missing onChange validation after array field helper me…

22868fb

…thods

test(FormApi): onChange validation after array field helper methods +…

7afd72a

… add missing moveFieldValues test

style(FormApi): Run linter

f4508c8

feat(FormApi): add helper function to validate a single field on a form

4aaecd9

test(FieldApi): onChange validation after array field helper methods

c679d6f

test(FieldApi): single field validation

e773f98

style: Run linter

cdc1fde

docs(FormApi): add validateField to references documentation

4759964

fulopkovacs reviewed May 18, 2024

View reviewed changes

packages/form-core/src/tests/FieldApi.spec.ts Outdated Show resolved Hide resolved

fulopkovacs mentioned this pull request May 18, 2024

Field Array Validation Not Refreshing Properly When Field is Removed #629

Closed

fulopkovacs reviewed May 18, 2024

View reviewed changes

packages/form-core/src/tests/FieldApi.spec.ts Outdated Show resolved Hide resolved

gutentag2012 added 3 commits May 20, 2024 14:43

chore: Remove log + only test

184a2e5

Merge main into fix/array-field-validation

5989407

chore: fix failing tests after merge

54867b5

Balastrong reviewed May 31, 2024

View reviewed changes

Balastrong mentioned this pull request May 31, 2024

feat(core): forward options from field to form methods #701

Merged

chore(core): prevent field validation from forcing the isTouched state

c87b69f

chore(core): fix tests

de8d8aa

fulopkovacs self-assigned this Jun 1, 2024

fulopkovacs requested changes Jun 1, 2024

View reviewed changes

gutentag2012 added 3 commits June 2, 2024 12:27

chore(core): remove redundant async notation

7c92b8d

chore(core): touch fields during validation again

dbeea0f

chore(core): add method to validate array fields sub-fields from a gi…

6412d2d

…ven index

gutentag2012 added 3 commits June 2, 2024 12:33

fix(core): validate array + array subfields when using helper functions

72816d6

test(core): adjust test to fixes in array helper function validation

60a2018

docs(core): add ref to new validateArrayFieldsStartingFrom function

43dff36

fulopkovacs approved these changes Jun 2, 2024

View reviewed changes

fulopkovacs merged commit 0201e65 into TanStack:main Jun 2, 2024
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix/array field validation #700

Fix/array field validation #700

gutentag2012 commented May 12, 2024

nx-cloud bot commented May 18, 2024 •

edited

Loading

Balastrong left a comment

gutentag2012 commented May 31, 2024

Balastrong commented May 31, 2024

gutentag2012 commented May 31, 2024

Balastrong commented May 31, 2024

gutentag2012 commented May 31, 2024

fulopkovacs left a comment

gutentag2012 commented Jun 2, 2024

gutentag2012 commented Jun 2, 2024

codecov bot commented Jun 2, 2024

fulopkovacs left a comment

	if (!this.state.meta.isTouched) return []

	try {
	this.form.validate(cause)
	} catch (_) {}

Fix/array field validation #700

Fix/array field validation #700

Conversation

gutentag2012 commented May 12, 2024

nx-cloud bot commented May 18, 2024 • edited Loading

☁️ Nx Cloud Report

Balastrong left a comment

Choose a reason for hiding this comment

gutentag2012 commented May 31, 2024

Balastrong commented May 31, 2024

gutentag2012 commented May 31, 2024

Balastrong commented May 31, 2024

gutentag2012 commented May 31, 2024

fulopkovacs left a comment

Choose a reason for hiding this comment

Notes on testing

gutentag2012 commented Jun 2, 2024

gutentag2012 commented Jun 2, 2024

codecov bot commented Jun 2, 2024

Codecov Report

fulopkovacs left a comment

Choose a reason for hiding this comment

nx-cloud bot commented May 18, 2024 •

edited

Loading